From 5ea7a97d58cf0b6d780ef71bb74455cb1f3e6cae Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Fri, 19 Feb 2016 11:02:17 -0800
Subject: [PATCH] Refactor links validation to not use PackageSet

Eventually we may not have the entire set of packages resident in memory, so
refactor the implementation to validate the `links` attribute in a demand driven
way rather than all at once up front.
---
 src/cargo/ops/cargo_rustc/context.rs |  3 ++
 src/cargo/ops/cargo_rustc/links.rs   | 49 +++++++++++++++++-----------
 src/cargo/ops/cargo_rustc/mod.rs     |  2 +-
 3 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/src/cargo/ops/cargo_rustc/context.rs b/src/cargo/ops/cargo_rustc/context.rs
index 40a0b1e6f..a3d7fbd7b 100644
--- a/src/cargo/ops/cargo_rustc/context.rs
+++ b/src/cargo/ops/cargo_rustc/context.rs
@@ -14,6 +14,7 @@ use super::TargetConfig;
 use super::custom_build::{BuildState, BuildScripts};
 use super::fingerprint::Fingerprint;
 use super::layout::{Layout, LayoutProxy};
+use super::links::Links;
 use super::{Kind, Compilation, BuildConfig};
 use super::{ProcessEngine, ExecEngine};
 
@@ -37,6 +38,7 @@ pub struct Context<'a, 'cfg: 'a> {
     pub compiled: HashSet<Unit<'a>>,
     pub build_config: BuildConfig,
     pub build_scripts: HashMap<Unit<'a>, Arc<BuildScripts>>,
+    pub links: Links<'a>,
 
     host: Layout,
     target: Option<Layout>,
@@ -94,6 +96,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
             compiled: HashSet::new(),
             build_scripts: HashMap::new(),
             build_explicit_deps: HashMap::new(),
+            links: Links::new(),
         })
     }
 
diff --git a/src/cargo/ops/cargo_rustc/links.rs b/src/cargo/ops/cargo_rustc/links.rs
index 060941346..779cd1373 100644
--- a/src/cargo/ops/cargo_rustc/links.rs
+++ b/src/cargo/ops/cargo_rustc/links.rs
@@ -1,38 +1,49 @@
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 
-use core::{PackageId, PackageSet};
+use core::PackageId;
 use util::CargoResult;
+use super::Unit;
 
-// Validate that there are no duplicated native libraries among packages and
-// that all packages with `links` also have a build script.
-pub fn validate(deps: &PackageSet) -> CargoResult<()> {
-    let mut map: HashMap<_, &PackageId> = HashMap::new();
+pub struct Links<'a> {
+    validated: HashSet<&'a PackageId>,
+    links: HashMap<String, &'a PackageId>,
+}
+
+impl<'a> Links<'a> {
+    pub fn new() -> Links<'a> {
+        Links {
+            validated: HashSet::new(),
+            links: HashMap::new(),
+        }
+    }
 
-    for dep in deps.packages() {
-        let lib = match dep.manifest().links() {
+    pub fn validate(&mut self, unit: &Unit<'a>) -> CargoResult<()> {
+        if !self.validated.insert(unit.pkg.package_id()) {
+            return Ok(())
+        }
+        let lib = match unit.pkg.manifest().links() {
             Some(lib) => lib,
-            None => continue,
+            None => return Ok(()),
         };
-        if let Some(prev) = map.get(&lib) {
-            let dep = dep.package_id();
-            if prev.name() == dep.name() && prev.source_id() == dep.source_id() {
+        if let Some(prev) = self.links.get(lib) {
+            let pkg = unit.pkg.package_id();
+            if prev.name() == pkg.name() && prev.source_id() == pkg.source_id() {
                 bail!("native library `{}` is being linked to by more \
                        than one version of the same package, but it can \
                        only be linked once; try updating or pinning your \
                        dependencies to ensure that this package only shows \
-                       up once\n\n  {}\n  {}", lib, prev, dep)
+                       up once\n\n  {}\n  {}", lib, prev, pkg)
             } else {
                 bail!("native library `{}` is being linked to by more than \
                        one package, and can only be linked to by one \
-                       package\n\n  {}\n  {}", lib, prev, dep)
+                       package\n\n  {}\n  {}", lib, prev, pkg)
             }
         }
-        if !dep.manifest().targets().iter().any(|t| t.is_custom_build()) {
+        if !unit.pkg.manifest().targets().iter().any(|t| t.is_custom_build()) {
             bail!("package `{}` specifies that it links to `{}` but does not \
-                   have a custom build script", dep.package_id(), lib)
+                   have a custom build script", unit.pkg.package_id(), lib)
         }
-        map.insert(lib, dep.package_id());
+        self.links.insert(lib.to_string(), unit.pkg.package_id());
+        Ok(())
     }
-
-    Ok(())
 }
diff --git a/src/cargo/ops/cargo_rustc/mod.rs b/src/cargo/ops/cargo_rustc/mod.rs
index 8f2b629de..8268eb50d 100644
--- a/src/cargo/ops/cargo_rustc/mod.rs
+++ b/src/cargo/ops/cargo_rustc/mod.rs
@@ -77,7 +77,6 @@ pub fn compile_targets<'a, 'cfg: 'a>(pkg_targets: &'a PackagesToBuild<'a>,
             }
         })
     }).collect::<Vec<_>>();
-    try!(links::validate(packages));
 
     let dest = if build_config.release {"release"} else {"debug"};
     let root = packages.packages().find(|p| {
@@ -179,6 +178,7 @@ fn compile<'a, 'cfg: 'a>(cx: &mut Context<'a, 'cfg>,
     let p = profile::start(format!("preparing: {}/{}", unit.pkg,
                                    unit.target.name()));
     try!(fingerprint::prepare_init(cx, unit));
+    try!(cx.links.validate(unit));
 
     let (dirty, fresh, freshness) = if unit.profile.run_custom_build {
         try!(custom_build::prepare(cx, unit))
-- 
2.30.2